webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (2)

50-50: Document protocol detection behavior.

Given that this change enables automatic protocol detection for S3 vaults, consider adding a code comment explaining this behavior to help future maintainers understand the purpose of passing WSClient.

+      // WSClient is used to detect if the S3 vault supports HTTPS by sending a test request
+      // before falling back to HTTP if necessary
       S3DataVault.create(remoteSource, ws)

---

Line range hint 41-64: Consider refactoring scheme handling for better maintainability.

The current implementation uses string comparisons for scheme detection. Consider extracting these into a sealed trait/enum for better type safety and maintainability.

Example refactor:

sealed trait VaultScheme {
  def value: String
}
object VaultScheme {
  case object S3 extends VaultScheme { val value = "s3" }
  case object HTTPS extends VaultScheme { val value = "https" }
  case object HTTP extends VaultScheme { val value = "http" }
  case object GS extends VaultScheme { val value = "gs" }
  case object File extends VaultScheme { val value = "file" }

  def fromString(scheme: String): Option[VaultScheme] = 
    values.find(_.value == scheme)
  
  def values: Seq[VaultScheme] = Seq(S3, HTTPS, HTTP, GS, File)
}

package.json (1)

126-126: Consider adding error handling to the startup script.

The new startf script combines cleanup and startup steps, which is helpful for development workflow. However, consider making it more robust:

-    "startf": "yarn rm-fossil-lock; yarn kill-listeners; rm -r webknossos-jni/target; yarn start",
+    "startf": "yarn rm-fossil-lock; yarn kill-listeners; rm -rf webknossos-jni/target 2>/dev/null || true; yarn start",

The changes:

• Added -f flag to rm to prevent errors if files don't exist
• Added 2>/dev/null || true to suppress errors and continue execution

test/backend/DataVaultTestSuite.scala (3)

141-149: Enhance data integrity verification in brotli-compression tests.

The current test only verifies the first 10 bytes of the decompressed data. Consider:

1. Verifying the complete data integrity
2. Adding test cases for different compression types
3. Testing large file handling
4. Adding partial read tests

---

180-203: Enhance directory listing test coverage.

Current tests could be more comprehensive. Consider adding test cases for:

1. Empty directories
2. Directories with exactly maxItems entries
3. Pagination scenarios
4. Special characters in paths
5. Nested directory structures

Also, consider parameterizing the tests with different maxItems values instead of hardcoding 3:

Seq(0, 1, 5, 10).foreach { maxItems =>
  s"list available directories with maxItems=$maxItems" in {
    // Test implementation
  }
}

---

Based on the code analysis, I can now generate the final response:

Test suite needs focus on HTTPS fallback mechanism and data vault interactions

The current test suite focuses on basic data vault operations but misses critical scenarios:

• The HTTPS fallback mechanism is not explicitly tested, despite being a core feature of HttpsDataVault
• No tests verify the automatic fallback to HTTPS when other protocols fail
• Missing edge cases for range requests and protocol switching

Suggested improvements:

• Add test cases for protocol fallback scenarios in DataVaultTestSuite.scala
• Include tests for supportsRangeRequests behavior in HttpsDataVault
• Test error handling during protocol transitions

🔗 Analysis chain

Line range hint 1-277: Consider restructuring tests for better organization and coverage.

While the test suite provides good basic coverage, consider:

1. Creating a dedicated test section for HTTPS fallback mechanism (PR's main objective)
2. Using shared test fixtures to reduce duplication
3. Organizing tests by feature rather than by request type
4. Adding integration tests with Hetzner Object Storage (mentioned in PR objectives)

Let's verify the test coverage:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any direct references to Hetzner storage in tests
rg -i "hetzner" --type scala

Length of output: 28

---

Script:

#!/bin/bash
# Let's check for any S3 or HTTPS fallback related code in the codebase
rg -A 5 "HttpsDataVault" --type scala

# Also check for any other test files related to data vaults
fd ".*Test.*\.scala$" | grep -i "vault"

Length of output: 5446

---

Script:

#!/bin/bash
# Let's check the implementation of HttpsDataVault to understand the fallback mechanism
ast-grep --pattern 'class HttpsDataVault $_' -A 30

# Also check for any S3 fallback related code
rg "fallback" --type scala

Length of output: 21184

webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (2)

Line range hint 131-138: Potential Unhandled Exceptions in getObjectSummaries

While fetching object summaries, exceptions other than NoSuchBucketException may not be adequately handled, potentially causing the application to crash on unexpected errors.

Implement comprehensive exception handling to manage all possible exceptions from listObjectsV2:

objectListing <- notFoundToFailure(client.listObjectsV2(listObjectsRequest).asScala).recoverWith {
  case exception: SomeSpecificExceptionType =>
    // Handle specific exception
    Fox.failure(exception)
}

---

232-233: Sequential Dependency on Protocol Determination

The construction of the S3 client depends on the result of determineProtocol, which might introduce latency due to the network call. This could be problematic in high-throughput scenarios.

Consider decoupling client creation from protocol determination or caching the result to minimize repeated network calls.

Configuration used: CodeRabbit UI
Review profile: CHILL